Cleanup remotefile resolver and delete internal/worker by lzap · Pull Request #2178 · osbuild/images

lzap · 2026-02-06T11:59:43Z

This builds on top of #1957 and fixes tests. On top of that, it adds a bunch of small improvements which are rather insignificant but nice to have.

This all is needed in order to use the (currently unused) resolver for GPG keys: osbuild/osbuild#2326

Additionally, once this is merged we should make use of it in composer, there seems to be a copy of this functionality:

https://github.com/osbuild/osbuild-composer/tree/main/internal/remotefile

The randomness in tests is solved in two commits, both will work or just one of them too (sort errors or just accept they come in random order).

Context was present but never used.

Then the code was not thread safe, but it is likely only used from one goroutine so it was never a problem. But I think using wait groups is actually nicer code.

What was undocumented was that call of Add after Finish would cause panic, I think it is fair to just document that and live with it. Any solution will make the code less readable than it can be and we do not need that robustness. Honestly, the way this is implemented is very simplistic and could be improved but this is not something we need today.

Then I dropped the buffering from the channel, this has no effect and it makes the code a bit confusing. Discuss, I can drop this if you feel strong about this.

Finally, the HTTP error codes were not handled so this would silently create empty files. I am not sure if this was desired behavior, can drop if it was.

I do have some other ideas in mind to further improve this, but I wanted to keep things simple. But just in case:

The code currently hardcodes the HTTP Client, which makes it tough to use HTTPS in the future. Ideally, HTTP client could be an argument passed into NewResolver. This usually makes testing easier, setting timeouts or TLS possible.
The code spawns all downloads immediately, there is no cap on how many can run in parallel. Since this is unused now and after integrating it with composer it is only used for a rare feature (file download, I guess) it might not be a problem, but HTTP servers typically limit amount of concurrent requests per client IP.
Early validation of URLs before goroutines are even spawned, not sure if that is worth the effort tho.

thozza

Thanks for taking this on. All the enhancements make sense, but I don't like the change to make the test cope with indeterminism when the results are actually deterministic.

pkg/remotefile/resolver_test.go

pkg/remotefile/resolver.go

lzap · 2026-02-09T11:06:11Z

Dropped the unnecessary test change.

Renamed queue to results which is short and representative.

Added one new commit that adds HTTP client as an optional argument. This is new but now that I look on it this can be also added later without breaking the API so I can drop this one.

pkg/remotefile/client.go

lzap · 2026-02-10T11:08:50Z

Yeah good catch, I meant to do this but forgot. Modified one test to accommodate for the change. Rebased.

thozza

I've added some nitpicks, but I won't bother you further.

Also, I really like the use of the functional options pattern. It looks great ❤️

pkg/remotefile/resolver.go

pkg/remotefile/client_test.go

The remotefile package provides a content resolver implementation, not a customization. Move it to the pkg/ directory, where also other content resolver packages live. Signed-off-by: Tomáš Hozza <thozza@redhat.com>

Rework the remotefile resolver to not embed the resolution error in the Spec definition, but instead return error from the resolver `Finish()` method. This is consistent with i.e. the container resolver. Additionally, don't use clienterrors package for errors. Signed-off-by: Tomáš Hozza <thozza@redhat.com>

This is a leftover from osbuild-composer split, while the authoritative copy still lives in osbuild-composer repository. The package is not needed in the images library.

To ensure deterministic error messages, sort the error messages before returning them and then remove consecutive duplicate error messages (which are unlikely but the call is cheap).

Previously, the context was completely unused. Let's use it to ensure that possible deadlines are respected.

lzap · 2026-02-11T13:20:17Z

Agreed on test and your point actually revealed that I accidentally implemented WithClient passing the client into the resolver, but what I meant was to pass HTTP Doer (HTTP transport client). This way a default one can be used, but also a custom TLS can be created. This was how I wanted it to be.

Added new commit that solves this:

remotefile: accept HTTP Doer instead of Client

This allows users to provide their own HTTP client implementation,
such as a custom transport or timeout. This was the idea in the first
place, but I forgot to implement it.

thozza

The code looks OK, but now I feel like the patch set became a spaghetti monster. Given that you've added new commits multiple times and there was not that much code to review and too many reviews, I would advice to squash changes that are basically fixups, with the original commits. I feel like this is a bit of a mess now.

Previously, the resolver was not concurrent safe. Let's make it so that multiple calls to Add and Finish can be made concurrently.

The results are being collected via a buffered channel which has a small size of 2. Collecting of results is very efficient compared to HTTP calls, therefore the chance that the channel will block is very low. While 2 will definitely not hurt, I find it confusing because the name of the field is "queue" which made me think of a queue of items to be processed. That is not the case here, it is queue of results.

These error codes were silently ignored.

I was a bit confused when I saw the queue, together with the buffered channel I made a wrong assumption.

This allows users to provide their own HTTP client implementation, such as a custom transport or timeout.

lzap · 2026-02-12T08:00:45Z

Squashed two commit pairs, so from 12 there are 10 commits. I think the rest is worth keeping separate.

lzap · 2026-02-13T06:54:22Z

Since you already commented in the original PR, @achilleas-k cheers!

lzap requested a review from a team as a code owner February 6, 2026 11:59

lzap requested review from achilleas-k, croissanne and supakeen February 6, 2026 11:59

thozza reviewed Feb 6, 2026

View reviewed changes

pkg/remotefile/resolver_test.go Outdated Show resolved Hide resolved

pkg/remotefile/resolver.go Outdated Show resolved Hide resolved

lzap force-pushed the resolver-cleanup branch from 461c735 to 2930004 Compare February 9, 2026 11:04

thozza reviewed Feb 9, 2026

View reviewed changes

pkg/remotefile/client.go Show resolved Hide resolved

lzap force-pushed the resolver-cleanup branch from 2930004 to 85fff07 Compare February 10, 2026 11:08

thozza previously approved these changes Feb 10, 2026

View reviewed changes

pkg/remotefile/resolver.go Outdated Show resolved Hide resolved

pkg/remotefile/client_test.go Outdated Show resolved Hide resolved

pkg/remotefile/client_test.go Outdated Show resolved Hide resolved

thozza and others added 5 commits February 11, 2026 14:09

Move remotefile package from pkg/customizations to pkg/

2097f06

The remotefile package provides a content resolver implementation, not a customization. Move it to the pkg/ directory, where also other content resolver packages live. Signed-off-by: Tomáš Hozza <thozza@redhat.com>

Delete internal/worker

76e9b15

This is a leftover from osbuild-composer split, while the authoritative copy still lives in osbuild-composer repository. The package is not needed in the images library.

remotefile: sort error messages

393fd00

To ensure deterministic error messages, sort the error messages before returning them and then remove consecutive duplicate error messages (which are unlikely but the call is cheap).

remotefile: propagate context to client

27e2613

Previously, the context was completely unused. Let's use it to ensure that possible deadlines are respected.

lzap dismissed thozza’s stale review via 54e643d February 11, 2026 13:19

lzap force-pushed the resolver-cleanup branch from 85fff07 to 54e643d Compare February 11, 2026 13:19

thozza reviewed Feb 12, 2026

View reviewed changes

lzap added 5 commits February 12, 2026 08:58

remotefile: make resolver concurrently safe

89de071

Previously, the resolver was not concurrent safe. Let's make it so that multiple calls to Add and Finish can be made concurrently.

remotefile: check status code of HTTP response

0370450

These error codes were silently ignored.

remotefile: rename the result channel properly

f9368af

I was a bit confused when I saw the queue, together with the buffered channel I made a wrong assumption.

remotefile: make the client configurable

ce356b3

This allows users to provide their own HTTP client implementation, such as a custom transport or timeout.

lzap force-pushed the resolver-cleanup branch from 54e643d to ce356b3 Compare February 12, 2026 07:59

thozza approved these changes Feb 12, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup remotefile resolver and delete internal/worker#2178

Cleanup remotefile resolver and delete internal/worker#2178
lzap wants to merge 10 commits intoosbuild:mainfrom
lzap:resolver-cleanup

lzap commented Feb 6, 2026 •

edited

Loading

Uh oh!

thozza left a comment

Uh oh!

Uh oh!

Uh oh!

lzap commented Feb 9, 2026

Uh oh!

Uh oh!

lzap commented Feb 10, 2026

Uh oh!

thozza left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lzap commented Feb 11, 2026

Uh oh!

thozza left a comment

Uh oh!

lzap commented Feb 12, 2026

Uh oh!

lzap commented Feb 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

lzap commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thozza left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lzap commented Feb 9, 2026

Uh oh!

Uh oh!

lzap commented Feb 10, 2026

Uh oh!

thozza left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lzap commented Feb 11, 2026

Uh oh!

thozza left a comment

Choose a reason for hiding this comment

Uh oh!

lzap commented Feb 12, 2026

Uh oh!

lzap commented Feb 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lzap commented Feb 6, 2026 •

edited

Loading